@epociask I think this is true right? cache and fallback don't make sense in keccak commitment case, since s3 is the main storage engine for keccak commitments? Unless there's a way to put a redis cache in front of the s3 reason for whatever reason?

@mcortesi how would a commitment ever be nil for keccak if we're returning up the call-stack via putWithKey and never touching downstream secondary storage insertions? If not mistaken the proposed changes are isomorphic with the existing implementation

@epociask the local variable commit which is then passed to HandleRedundanWrites() is only ever set as the result of putWithoutKey().

@mcortesi that is the intended behavior - we treat S3 or keccak256 commitment mode with less prestige since our initial reasons for supporting it was to enable a migration path from keccak to eigenda commitment modes for OP Stack

@epociask i understand that...

but if i call Put() with commitments.OptimismKeccak mode; and cache or fallback enabled you end up calling HandleRedudndateWrites() with nil, which will fail or have some weird behaviour.

Again, too much discussion for just some clean up really.

To me, the change means safer/more clear code. But if you don't agree, or feel it's unnecessary and are happy sending nil to function that don't expect that 🤷‍♂️. I think we are spending more time writing comments that coding it :)

Hey @mcortesi just catching up here, sorry for the delay. I agree with the refactor because I think it makes the code clearer and less bug-prone to future changes, but I disagree with your reasoning. You must have missed the return statement in the OptimismKeccak case?

	case commitments.OptimismKeccak: // caching and fallbacks are unsupported for this commitment mode
		return r.putWithKey(ctx, key, value)

Hence

but if i call Put() with commitments.OptimismKeccak mode; and cache or fallback enabled you end up calling HandleRedudndateWrites() with nil, which will fail or have some weird behaviour.

is just not possible.

But please just fix the lint error, and remove the FIXME in the code which is confusing, and then we can merge this :)